add specs for CGI.escapeURIComponent
#1080
Conversation
| CGI.escapeURIComponent("whatever".force_encoding("ASCII-8BIT")).encoding.should == Encoding::ASCII_8BIT | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
suggestion: we could add some other cases:
- for implicit type conversion of an argument (to String with method
#to_str) - for ascii-compatible String and ascii-not-compatible (it treats a String as a bytes sequence and converts bytes, not characters)
There was a problem hiding this comment.
I'm a bit uncertain here. I added a spec for the implicit type conversion, but I'm not sure if I understood it correctly what you suggested. Please let me know if I misunderstood.
I could use a pointer regarding the ascii-not-compatible string idea... how could I approach this? Thanks for your help, I appreciate it!
There was a problem hiding this comment.
Regarding implicit conversion - you are correct. Actually you may look for similar test cases, e.g. here:
spec/core/basicobject/instance_eval_spec.rb
Lines 287 to 301 in db5f026
There was a problem hiding this comment.
for ascii-compatible String and ascii-not-compatible (it treats a String as a bytes sequence and converts bytes, not characters)
I could use a pointer regarding the ascii-not-compatible string idea... how could I approach this?
I meant that most of the test cases use ASCII characters. But the method actually handles multibyte encodings, e.g. UTF-8/UTF-16/UTF-32 and just converts their bytes one by one (ignoring "characters" that could contain several bytes):
CGI.escapeURIComponent("β")
# => "%CE%B2"
"β".bytes.map {|c| c.to_s(16) }
# => ["ce", "b2"]| end | ||
|
|
||
| it "nil raises a TypeError" do | ||
| -> { CGI.escapeURIComponent(nil) }.should raise_error(TypeError) |
There was a problem hiding this comment.
suggestion: usually it makes sense to add exception message when it's stable among supported Ruby versions.
There was a problem hiding this comment.
Here I'm a bit uncertain too... I now added an additional assertion for the error message, is this what you meant? If not, let me know so I can fix it.
There was a problem hiding this comment.
Yeah, it's what I meant. But it could be simplified a bit as far as raise_error accepts String or Regexp to match an exception message as a second argument, e.g.:
-> { }.should raise_error(ArgumentError, /duplicate/)
-> { }.should raise_error(TypeError, "non-hash or symbol given")There was a problem hiding this comment.
Thanks for that hint. I managed to miss that one in CONTRIBUTING.md, apologies. Will fix it.
|
Looks good!
I believe it's completely OK. |
|
@andrykonchin Thanks a lot for your precious feedback. I pushed some commits which should address at least some of your feedbacks. |
|
Will do it myself. |
|
Thank you! |
Hi again 👋 (it's been a while)
This PR tries to address the first part of
CGI.escapeURIComponentandCGI.unescapeURIComponentare added.[Feature #18822]
from #1016 .
I mainly took over the tests from the implementation and added a few more.
Let me know what makes sense and what doesn't.
I can add specs for
.unescapeURIComponentin another PR, if that's OK?